Activated flag strictNullInputTypes on Angular 21 - #7151
Conversation
…several templates. Replacing with signals
cdavalos7
changed the title
arcra
left a comment
arcra
left a comment
There was a problem hiding this comment.
I think at least we should aim to remove the ! characters, possibly define default values.
| }); | ||
| }) | ||
| ) | ||
| this.numAlerts = this.store.selectSignal(getNumAlerts); |
There was a problem hiding this comment.
No need to change it in this PR if you prefer the alternative of adding the type to the variables, but in the future, the recommended pattern is to use inject() and direct assignment of the signals outside of the constructor (and the constructor can likely be removed).
| }) | ||
| export class AlertsContainer { | ||
| readonly numAlerts$; | ||
| readonly numAlerts; |
There was a problem hiding this comment.
See my comment below, but if we're not assigning something directly to these, we should specify the type for each of them (although at that point, then I might prefer the direct assignment).
AFAIU, with a direct assignment at declaration, the static analyzer can infer the type, and thus, also signal when the variable is not used as it should be used. I don't know if it's the same for these without a type. I would imagine that the static analyzer would consider them to have type any, which would not be helpful to catch errors.
Although... since they're declared as "readonly" ... it's possible that the analyzer knows how to infer the type from the assignment in the constructor... I don't really know (google says that that's not the case).
I recognize this is also not introduced here, but that's how it was before, so maybe it's ok to leave it as is, but in the future, let's prefer assignment at declaration.
| ) | ||
| this.numAlerts = this.store.selectSignal(getNumAlerts); | ||
| this.alertsBreakdown = this.store.selectSignal( | ||
| createSelector(getAlertsBreakdown, (alertsBreakdown) => { |
There was a problem hiding this comment.
No need to change it in this PR, but wondering what you think about these being computed signals instead of a selector defined inline?
I guess we'd need to first have a signal for the existing selector and then a separate computed signal for the thing we actually care about... which is a bit annoying.
| ) | ||
| ) | ||
| this.tensorDebugMode = this.store.selectSignal( | ||
| createSelector(getFocusedExecutionData, (execution: Execution | null) => { |
There was a problem hiding this comment.
This one should have more clearly been a computed signal, since we already have a signal for the selector used here.
(But again, no need to change it here. I understand you might have just got for a simple code update, especially considering the amount of files modified.)
| export class GraphComponent { | ||
| @Input() | ||
| opInfo!: GraphOpInfo; | ||
| opInfo!: GraphOpInfo | null; |
There was a problem hiding this comment.
Why does this variable have a !, and then also a | null type?
Is the ! character saying that we're expecting it to never be null or undefined? I think this needs to be updated?
| readonly focusedSourceLineSpec; | ||
|
|
||
| readonly useDarkMode$: Observable<boolean>; | ||
| readonly useDarkMode; |
There was a problem hiding this comment.
This one had a type, and it was removed. I think this is a regression. This should be Signal ?
| readonly mode; | ||
| readonly xAxisType; | ||
| readonly showFullWidth; | ||
| isPinned!: Signal<boolean>; |
There was a problem hiding this comment.
Again for these ! symbols. Let's set a default value? Here and above.
| this.steps$ = this.store.select(getMetricsImageCardSteps, this.cardId); | ||
| const steps$ = this.store.select(getMetricsImageCardSteps, this.cardId); | ||
| this.steps = toSignal(steps$, { | ||
| injector: this.injector, |
There was a problem hiding this comment.
I'm curious, why was it necessary to specify this?
| ); | ||
| this.showPaginationControls$ = this.numPages$.pipe( | ||
| map((numPages) => numPages > 1) | ||
| this.isGroupExpanded = toSignal(this.isGroupExpanded$, { |
There was a problem hiding this comment.
A couple other examples where these should have been computed signals.
If you'd like to make these changes, we can also have separate PRs with smaller scope. e.g. updating only one subdirectory at a time, or something.
| this.notificationNotes = toSignal(notificationNotes$, { | ||
| requireSync: true, | ||
| }); | ||
| this.hasUnreadMessages = computed(() => |
There was a problem hiding this comment.
Huh... this one did become a computed signal...
2 participants
Motivation for features / changes
Enable
strictNullInputTypesto strengthen Angular template type safety, catch nullable input bindings at build time, and prevent UI errors caused by loading or unavailable data. The change also modernizes synchronous NgRx selector bindings with Angular signals and reduces redundant reactive subscriptions.Technical description of changes
strictNullInputTypes: truein tsconfig.json.selectSignalortoSignal({requireSync: true}).